Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing missing prefix for extraInputs #899

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perry-mitchell
Copy link

Issue

NA.

Description of changes

The extraInputs option is actually input.extraInputs according to the configmap template.

Checklist

  • Added/modified documentation as required (such as the README.md for modified charts)
  • Incremented the chart version in Chart.yaml for the modified chart(s)
  • Manually tested. Describe what testing was done in the testing section below
  • Make sure the title of the PR is a good description that can go into the release notes

Testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The `extraInputs` option is actually `input.extraInputs` according to the configmap template.
@@ -39,7 +39,7 @@ helm delete aws-for-fluent-bit --namespace kube-system
| `service.parsersFiles` | List of available parser files | `/fluent-bit/parsers/parsers.conf` |
| `service.extraParsers` | Adding more parsers with this value | `""` |
| `input.*` | Values for Kubernetes input | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you fix the language above too? This is the kubernetes tail log file input.

@@ -39,7 +39,7 @@ helm delete aws-for-fluent-bit --namespace kube-system
| `service.parsersFiles` | List of available parser files | `/fluent-bit/parsers/parsers.conf` |
| `service.extraParsers` | Adding more parsers with this value | `""` |
| `input.*` | Values for Kubernetes input | |
| `extraInputs` | Append to existing input with this value | `""` |
| `input.extraInputs` | Append to existing input with this value | `""` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what this is meant to do is to append to the kubernetes tail input. Because when I do a simple silly test:

$ helm template --dry-run . --set input.extraInputs="[INPUT]somekey someval"
...

  fluent-bit.conf: |
    [SERVICE]
        Parsers_File /fluent-bit/parsers/parsers.conf

    [INPUT]
        Name              tail
        Tag               kube.*
        Path              /var/log/containers/*.log
        DB                /var/log/flb_kube.db
        Parser            docker
        Docker_Mode       On
        Mem_Buf_Limit     5MB
        Skip_Long_Lines   On
        Refresh_Interval  10
        [INPUT]somekey someval

The new data is added indented to the existing tail input. So you could use this to set other values in it, like for example Ignore_Older:

$ helm template --dry-run . --set input.extraInputs="Ignore_Older 1d"
  fluent-bit.conf: |
    [SERVICE]
        Parsers_File /fluent-bit/parsers/parsers.conf

    [INPUT]
        Name              tail
        Tag               kube.*
        Path              /var/log/containers/*.log
        DB                /var/log/flb_kube.db
        Parser            docker
        Docker_Mode       On
        Mem_Buf_Limit     5MB
        Skip_Long_Lines   On
        Refresh_Interval  10
        Ignore_Older 1d

I think this was added because the input.* only allows you to customize a few things: https://github.com/aws/eks-charts/blob/master/stable/aws-for-fluent-bit/values.yaml#L29

So basically, the name here is wrong/misleading. It should be something like input.extraSettings.

I think the additionalInputs key is for adding other other inputs.

I don't know how this works- can we bump the chart version and change the setting name?

If not, can you please make the help text here much more clear: "Append other config options to the kubernetes tail input" something like that.

Copy link
Author

@perry-mitchell perry-mitchell Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PettitWesley - While I understand your points, I simply wanted to correct a typo in the readme. The functionality and naming therein should be out of scope for this, right? At least people working with the current version will then not make the same mistake I did, as the name will be correct.

Imo all of the names need to be reworked, because they're not consistent and are all confusing:

  • service.extraParsers
  • input.*
  • extraInputs
  • additionalInputs
  • filter.*
  • filter.extraFilters
  • additionalFilters

Unfortunately I don't have time right now to devote to such a PR, however 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I still think the language in your PR needs to be edited even just for this option. It should be clear what "existing input" means.

Append to existing kubernetes log file tail input with this value

would be prefered by me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is also another issue here that the CI only lets you merge updates that change the chart version, which I'll have to look into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants